Make Teleprompt.compile more typing friendly.#9013
Make Teleprompt.compile more typing friendly.#9013TyTodd wants to merge 3 commits intostanfordnlp:mainfrom
Teleprompt.compile more typing friendly.#9013Conversation
…racks the type exact type of the student module passed in
|
Thanks @TyTodd! Is it hard to have |
There was a problem hiding this comment.
Pull Request Overview
This pull request introduces improved type safety to the DSPy teleprompt system by adding generic type variables. The main changes include introducing TypeVar("M", bound=Module) to preserve the specific type of the student module through the compile method, replacing previous Module or Any return types.
- Added
TypeVarimports and definedM = TypeVar("M", bound=Module)in multiple teleprompt files - Updated
compilemethod signatures to use generic typeMinstead ofModuleorAnyfor better type preservation - Applied consistent code formatting (line breaks, spacing) across multiple files
Reviewed Changes
Copilot reviewed 6 out of 6 changed files in this pull request and generated 4 comments.
Show a summary per file
| File | Description |
|---|---|
| dspy/teleprompt/teleprompt.py | Added TypeVar M and updated base Teleprompter.compile signature to use generic type |
| dspy/teleprompt/simba.py | Added TypeVar M, updated SIMBA.compile signature, and reformatted docstrings |
| dspy/teleprompt/mipro_optimizer_v2.py | Added TypeVar M, updated MIPROv2.compile signature, and reformatted long lines |
| dspy/teleprompt/grpo.py | Added TypeVar M, updated GRPO.compile signature, and reformatted long lines/assertions |
| dspy/teleprompt/bootstrap_finetune.py | Added TypeVar M and updated FinetuneTeleprompter.compile signature |
| dspy/teleprompt/bettertogether.py | Added TypeVar M, updated BetterTogether.compile signature, and reformatted code |
Comments suppressed due to low confidence (3)
dspy/teleprompt/bettertogether.py:7
- The import
from dspy.primitives.module import Moduleon line 7 is redundant since line 17 already importsfrom dspy.primitives import Module. One of these imports should be removed to avoid duplication.
from dspy.primitives.module import Module
dspy/teleprompt/bettertogether.py:90
- The return type of
_run_strategiesshould beMinstead ofModuleto maintain consistency with the generic type used in thecompilemethod signature. This would ensure the type is preserved through internal method calls.
def _run_strategies(self, parsed_strategy, student, trainset, valset_ratio) -> Module:
dspy/teleprompt/bettertogether.py:128
- The return type of
_compile_prompt_optimizershould use the generic typeMinstead ofModulefor consistency with the type safety improvements introduced in this PR.
def _compile_prompt_optimizer(self, student, trainset, valset_ratio) -> Module:
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
| def compile( | ||
| self, | ||
| student: M, | ||
| *, | ||
| trainset: list[Example], | ||
| teacher: Module | None = None, | ||
| valset: list[Example] | None = None, | ||
| **kwargs, | ||
| ) -> M: |
There was a problem hiding this comment.
Overridden method signature does not match call, where it is passed too many arguments. Overriding method method BootstrapFinetune.compile matches the call.
Overridden method signature does not match call, where it is passed too many arguments. Overriding method method BootstrapFinetune.compile matches the call.
Overridden method signature does not match call, where it is passed an argument named 'trainset'. Overriding method method BootstrapFinetune.compile matches the call.
Overridden method signature does not match call, where it is passed an argument named 'trainset'. Overriding method method BootstrapFinetune.compile matches the call.
Overridden method signature does not match call, where it is passed an argument named 'teacher'. Overriding method method BootstrapFinetune.compile matches the call.
| def compile( | ||
| self, | ||
| student: Module, | ||
| student: M, | ||
| trainset: list[Example], | ||
| strategy: str = "p -> w -> p", | ||
| valset_ratio = 0.1, | ||
| ) -> Module: | ||
| valset_ratio=0.1, | ||
| ) -> M: |
There was a problem hiding this comment.
This method requires at least 3 positional arguments, whereas overridden Teleprompter.compile requires 2.
| def compile(self, student: M, trainset: list[Example], teacher: Module | list[Module] | None = None) -> M: | ||
| # TODO: Print statements can be converted to logger.info if we ensure | ||
| # that the default DSPy logger logs info level messages in notebook | ||
| # environments. |
There was a problem hiding this comment.
This method requires at least 3 positional arguments, whereas overridden Teleprompter.compile requires 2.
| def compile(self, student: M, trainset: list[Example], teacher: Module | list[Module] | None = None) -> M: | |
| # TODO: Print statements can be converted to logger.info if we ensure | |
| # that the default DSPy logger logs info level messages in notebook | |
| # environments. | |
| def compile(self, student: M, trainset: list[Example], **kwargs) -> M: | |
| # TODO: Print statements can be converted to logger.info if we ensure | |
| # that the default DSPy logger logs info level messages in notebook | |
| # environments. | |
| teacher = kwargs.get('teacher', None) |
| def compile( | ||
| self, | ||
| student: Module, | ||
| student: M, | ||
| trainset: list[Example], | ||
| teacher: Module | list[Module] | None = None, | ||
| valset: list[Example] | None = None, | ||
| **kwargs, | ||
| ) -> Module: | ||
| logger.info("Starting the GRPO compilation process... The LM(s) for the student program will be updated in place at the end of the training.") | ||
| ) -> M: |
There was a problem hiding this comment.
This method requires at least 3 positional arguments, whereas overridden Teleprompter.compile requires 2.
Thanks for your response! It's not really hard. It's more of a taste thing I guess. |
| launched_flag = False | ||
|
|
||
| for ind, step_code in enumerate(parsed_strategy): | ||
| current_strategy = self.STRAT_SEP.join(parsed_strategy[:ind + 1]) |
There was a problem hiding this comment.
Let's not include unrelated changes
There was a problem hiding this comment.
Must be my formatter. Let me see if I can undo the other formatting changes.
|
I see, using base class in type hint is very common but I also understand where you come from. @okhat @chenmoneygithub what do you think about using generic in the type hint of .compile? |
I replaced
student: ModuleinTeleprompt.compile's signature withstudent: M, a TypeVar that tracks the type and returns that same typeI also applied this signature to all other compilers in the SDK.
It doesn't change the functionality of anything at all. My main reason for adding this is I (and others) often add custom functionality to
dspy.Modules and its just convenient to have the type checker still recognize those functions and attributes on the optimized program after running compile.For example, in this script the type checker will not highlight
weather_functionbecause the custom function is forgotten when the new program is returned fromcompile. With this new commit that is no longer the case.